#3490 removing Content-Type and Transfer-Encoding headers on redirect #3493
Conversation
|
I would like to see tests for this please! ✨ |
|
I was on it too. Did not find a way to properly add tests though since httpbin does not seem to be able to post to an endpoint and respond with a 301/302. Am I wrong on this one? Anyway since @nateprewitt is on it I will not open my PR ;) |
|
On my version of the fix (kennethreitz/requests@master...saveman71:master) I moved this line a little higher and removed the try/catch below as it seems the general way (at least in this file) is to check the existence of the key prior to the deletion. |
nateprewitt
force-pushed
the
3490_drop_headers
branch
from
aaa1294 to
20a2b5a
Compare
|
@saveman71, sorry, I didn't mean to jump on your PR. I wasn't sure if anyone had started working on it. @Lukasa I added a test to check the header removal and empty body. |
|
@nateprewitt no worries at all, I should have stated I started working on it. I was suggesting that merely because it's already been like that here and here for example. On a side note, 👍 for a separate PR. |
|
@saveman71, I only bring up the optimization because @sigmavirus24 has brought it up before. If I'm misunderstanding his original intent though, I vote we completely remove the declaration, in favor of Edit: Clarity and Link |
nateprewitt
force-pushed
the
3490_drop_headers
branch
3 times, most recently
from
0ef19b9 to
394b353
Compare
|
Quick semi-unrelated question, what would be the timeline of a release on PyPI after this gets merged? cc @Lukasa |
|
We'd be aiming to release this early next week if we can merge it this week. |
|
To be clear, this is on my queue, but I'll review tomorrow. |
| # https://github.com/kennethreitz/requests/issues/3490 | ||
| purged_headers = ('Content-Length', 'Content-Type', 'Transfer-Encoding') | ||
| for header in purged_headers: | ||
| if header in prepared_request.headers: |
There was a problem hiding this comment.
I know I wrote this poorly the first time, but why don't we do:
prepared_request.headers.pop('Content-Length', None)
prepared_request.headers.pop('Content-Type', None)Or if you really want to shave the yak
for part in ('Length', 'Type'):
prepared_request.headers.pop('Content-' + part, None)There was a problem hiding this comment.
Just to be clear, I think the second suggestion is very bad, but I realize some people might actually prefer that.
There was a problem hiding this comment.
I originally used pop over del when writing it, but reverted to keep with the surrounding code. If you're onboard with using pop instead, let's do it.
nateprewitt
force-pushed
the
3490_drop_headers
branch
from
394b353 to
5c16e8b
Compare
| @@ -221,6 +221,23 @@ def test_http_303_doesnt_change_head_to_get(self, httpbin): | |||
| assert r.history[0].status_code == 303 | |||
| assert r.history[0].is_redirect | |||
|
|
|||
| def test_header_and_body_removal_on_redirect(self, httpbin): | |||
| purged_headers = ('Content-Length', 'Content-Type', 'Transfer-Encoding') | |||
There was a problem hiding this comment.
This test doesn't actually cover 'Transfer-Encoding' since 'Content-Length' is present. I can add a smaller separate test for 'Transfer-Encoding' if it's deemed that we definitely want to remove it.
This does ensure that as long as purged_headers contains 'Transfer-Encoding' in resolve_redirects, that it will be removed if it occurs. It just won't fail for accidental removal from the purged_headers tuple.
There was a problem hiding this comment.
Transfer-Encoding should be easily covered: a generator body will trigger it.
There was a problem hiding this comment.
Transfer-Encoding test code added.
nateprewitt
force-pushed
the
3490_drop_headers
branch
from
5c16e8b to
5f59dd9
Compare
| assert next_resp_tran.request.body is None | ||
| for header in purged_headers: | ||
| assert header not in next_resp.request.headers | ||
| assert header not in next_resp_tran.request.headers |
There was a problem hiding this comment.
Can we split this into two tests that look very similar, rather than have one really complex test?
There was a problem hiding this comment.
Sure thing. Tests split up now.
nateprewitt
force-pushed
the
3490_drop_headers
branch
from
5f59dd9 to
e50c61b
Compare
|
Alright, fab, I'm happy with this. @sigmavirus24 you ok with us merging this for 2.11.1? |
| req = requests.Request('POST', httpbin('post'), data=(b'x' for x in range(1))) | ||
| prep = ses.prepare_request(req) | ||
| assert 'Transfer-Encoding' in prep.headers | ||
| resp = ses.send(prep) |
There was a problem hiding this comment.
I reduced the range call to size 1 because we were hitting this bug when sending more than one chunk. After some more testing, this only passes most of the time, which likely isn't something we want to introduce.
The failures seem random and are tied specifically to the httpbin module. Calling the live http://httpbin.org/post endpoint won't produce the failure.
As I see it, two options are either calling the httpbin.org/post endpoint directly (probably not the best solution) or adding a try/except around resp = ses.send(prep) and manually creating the Response on failure.
Thoughts?
There was a problem hiding this comment.
There's a question about whether we should just manually create the response unconditionally, to be honest. That might be the most sensible thing to do.
There was a problem hiding this comment.
Alright, I'll make a note and create the Response manually for this.
There was a problem hiding this comment.
Ok, test updated with manual Response. If it looks good I'll squash things down.
nateprewitt
force-pushed
the
3490_drop_headers
branch
from
3acd5a5 to
4d5091c
Compare
|
Alright, all is well. @sigmavirus24, you want this in 2.11.1? |
|
👍 |
This will remove additional headers (
Content-TypeandTransfer-Encoding) from non-temporary/non-permanent redirects inresolve_redirectsto address #3490.This functionality isn't currently being tested but I'm happy to add a test or two for it if desired.